-
Notifications
You must be signed in to change notification settings - Fork 262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: One-click unsubscribe #8438
Conversation
src/components/ThreadEnvelope.vue
Outdated
|
||
await unsubscribe(this.envelope.databaseId) | ||
} catch (error) { | ||
logger.error('Could one-click unsubscribe', { error }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could not
one-click unsubscribe ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works as expected (I think).
The unsubscribe landing page is not shown (4. wrong in the test desc?) and the list is unsubscribed.
Personally, I would appreciate a little feedback, the unsubscribe was send.
My bad. Copied over the test steps from #8395. Will add a confirmation toast. Good idea! |
Tested, worked fine, cool stuff! Two nitpicks and a bug:
Alternatively add a few pixels of margin to the button:
I'd hide it for now and fix it at a later date. |
See #5387 and #5387 (comment) (bottom). The button was placed there intentionally.
It uses the default for success toasts: https://github.com/nextcloud/nextcloud-dialogs/blob/c16397fe19f43a4def95ba1dcab13ebf8929232c/lib/toast.ts#L45-L48
Tracked in #8191 |
Cool 👍
To comply with the RFC, we need to send the List-Unsubscribe-Post key/value pair to the unsubscribe url. |
Thanks @kesselb. Didn't see any header values in the messages I tested with. |
To add more fun:
Possible library to validate DKIM: https://github.com/pimlie/php-dkim (horde already has a couple of methods to fetch and normalize headers, maybe we don't need an additional lib). Plugin for Roundcube: https://github.com/pimlie/authres_status (very helpful readme with lots of information). |
efebc54
to
3289839
Compare
@kesselb does your rebase mean this is done and ready to review? :) |
3289839
to
e44f17c
Compare
if (str_starts_with($header->url, 'mailto')) { | ||
$this->unsubscribeMailto = $header->url; | ||
break; | ||
$dkimSignatureHeader = $parsedHeaders->getHeader('dkim-signature'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: early return would lead to better readability here and save about 3 brackets 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I will take that into account for the follow-up to add the DKIM signature validation.
@ChristophWurst ready to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 latest additions look good!
Co-Authored-By: Daniel Kesselberg <[email protected]> Signed-off-by: Christoph Wurst <[email protected]>
e44f17c
to
f2b61b3
Compare
Fixes #5387
Close #5387
Refinement of #8395 for services that support one-click unsubscribe.
https://datatracker.ietf.org/doc/html/rfc8058
How to test
List-Unsubscribe: <http…>
andList-Unsubscribe-Post=List-Unsubscribe=One-Click
headers)